fix: prevent shell options from environment variables - #1508
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
MuhammadNiazAli
left a comment
MuhammadNiazAli
left a comment
There was a problem hiding this comment.
Straightforward and well-scoped fix. Removing 'prefix', 'postfix', and 'shell' from ENV_OPTS in core.ts is the actual security-relevant change it stops those three from being settable via ZX_* environment variables, which closes an injection vector (an attacker controlling the environment could otherwise redirect script execution to an arbitrary shell/prefix without touching the CLI invocation itself).
The docs are updated consistently across both files:
cli.md explicitly calls out the "for security reasons" rationale and removes the outdated ZX_SHELL='/bin/bash' env var example while keeping the CLI-flag equivalent.
shell.md correctly drops the "or [envars]" link and the now-invalid ZX_SHELL=/bin/zsh bash example, leaving JS API and CLI flags as the only documented ways to set shell.
Nothing looks half-done the docs no longer reference the removed capability anywhere in the shown diff, and the code change (removing 3 items from a Set) is minimal and low-risk, with no other logic depending on those ENV_OPTS entries visible here.
One thing I can't verify from the diff: are there existing tests that assert ZX_SHELL/ZX_PREFIX/ZX_POSTFIX do work (i.e., tests that would now need updating/removing), or a test confirming they're now correctly ignored? Worth checking the test suite covers the negative case, otherwise this could regress silently.
3 participants
Fixes #1435.
This change prevents ZX_SHELL, ZX_PREFIX, and ZX_POSTFIX from being used to configure shell execution through environment variables.
Changes
Removed shell, prefix, and postfix from ENV_OPTS in src/core.ts.
Updated docs/cli.md to document the security restriction.
Removed the ZX_SHELL environment-variable example from docs/shell.md.
Existing CLI and JavaScript API configuration remains available.
Testing
The relevant behavior was manually verified:
ZX_PREFIX is no longer loaded.
ZX_POSTFIX is no longer loaded.
ZX_SHELL is no longer loaded from the environment.
Other supported environment variables continue to work.
The full local test/pre-push suite could not be completed in my Windows environment due to environment-specific failures in the size, license, and circular-dependency checks.